Add electrum support to `lightning-transaction-sync` #2685

tnull · 2023-10-25T14:40:37Z

Closes #2010.

This PR implements an ElectrumSyncClient that allows syncing LDK via the Confirm/Filter interfaces based on BDK's electrum-client crate.

We furthermore improve the logging in EsploraSyncClient and ElectrumSyncClient, and extend our test coverage.

~~Currently in draft as I want to do some further cleanup and experimentation.~~

codecov-commenter · 2023-10-27T10:29:13Z

Codecov Report

Attention: 15 lines in your changes are missing coverage. Please review.

Comparison is base (70ea110) 88.50% compared to head (31ea90e) 88.53%.

Files	Patch %	Lines
lightning/src/chain/channelmonitor.rs	0.00%	6 Missing ⚠️
lightning/src/chain/onchaintx.rs	0.00%	5 Missing ⚠️
lightning/src/chain/chainmonitor.rs	0.00%	3 Missing ⚠️
lightning/src/ln/channel.rs	83.33%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2685      +/-   ##
==========================================
+ Coverage   88.50%   88.53%   +0.02%     
==========================================
  Files         113      113              
  Lines       89323    89334      +11     
  Branches    89323    89334      +11     
==========================================
+ Hits        79055    79090      +35     
+ Misses       7890     7875      -15     
+ Partials     2378     2369       -9

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tnull · 2023-10-30T09:13:08Z

Did some further cleanup, should be ready for review now.

jkczyz

Still have a bit to get through

lightning-transaction-sync/src/esplora.rs

lightning-transaction-sync/tests/integration_tests.rs

lightning-transaction-sync/src/electrum.rs

lightning-transaction-sync/tests/integration_tests.rs

lightning-transaction-sync/src/electrum.rs

lightning-transaction-sync/src/esplora.rs

jkczyz

Mostly looks good. Feel free to squash fixups. Will take a final pass after.

lightning-transaction-sync/src/electrum.rs

lightning-transaction-sync/tests/integration_tests.rs

tnull · 2023-11-08T15:51:28Z

Squashed previous fixups and addressed the pending comments.

jkczyz

LGTM after a couple minor comments

lightning-transaction-sync/src/electrum.rs

tnull · 2023-11-10T08:49:25Z

Squashed fixups and included the following changes:

diff --git a/lightning-transaction-sync/src/electrum.rs b/lightning-transaction-sync/src/electrum.rs
index a37d017fd..a002fc8e2 100644
--- a/lightning-transaction-sync/src/electrum.rs
+++ b/lightning-transaction-sync/src/electrum.rs
@@ -320,13 +320,13 @@ where
                                                        if let Some(history) = script_history.iter()
                                                                .filter(|h| h.tx_hash == txid).max_by_key(|x| x.height)
-                                                               {
-                                                                       let prob_conf_height = history.height;
-                                                                       let block_header = self.client.block_header(
-                                                                               prob_conf_height as usize)?;
-                                                                       if block_header.block_hash() == block_hash {
-                                                                               // Skip if the tx is still confirmed in the block in question.
-                                                                               continue;
-                                                                       }
+                                                       {
+                                                               let prob_conf_height = history.height;
+                                                               let block_header = self.client.block_header(
+                                                                       prob_conf_height as usize)?;
+                                                               if block_header.block_hash() == block_hash {
+                                                                       // Skip if the tx is still confirmed in the block in question.
+                                                                       continue;
                                                                }
+                                                       }
                                                }
                                        }
@@ -404,11 +404,6 @@ where
                for mut bytes in merkle_res.merkle {
                        bytes.reverse();
-                       let next_hash = Sha256d::from_slice(&bytes).map_err(|_| {
-                               log_error!(self.logger,
-                                       "Failed due to the server sending us bogus transaction data. This should not happen. Please verify server integrity."
-                               );
-                               InternalError::Failed
-                       })?;
-
+                       // unwrap() safety: `bytes` has len 32 so `from_slice` can never fail.
+                       let next_hash = Sha256d::from_slice(&bytes).unwrap();
                        let (left, right) = if index % 2 == 0 {
                                (cur, next_hash)

lightning-transaction-sync/Cargo.toml

lightning-transaction-sync/src/esplora.rs

lightning-transaction-sync/tests/integration_tests.rs

lightning-transaction-sync/src/electrum.rs

tnull · 2023-11-15T16:57:14Z

Moved logging and updating SyncState back to the call sites and renamed the method:

> git diff-tree -U2 a48cf50b8 d609b5d37

diff --git a/lightning-transaction-sync/src/electrum.rs b/lightning-transaction-sync/src/electrum.rs
index 9d39ed87b..4dbd53609 100644
--- a/lightning-transaction-sync/src/electrum.rs
+++ b/lightning-transaction-sync/src/electrum.rs
@@ -113,7 +113,9 @@ where
                                                        // Double-check the tip hash. If it changed, a reorg happened since
                                                        // we started syncing and we need to restart last-minute.
-                                                       if self.check_has_chain_moved(&mut sync_state, &mut tip_header,
-                                                               &mut tip_height)?
+                                                       if self.check_update_tip(&mut tip_header, &mut tip_height)?
                                                        {
+                                                               log_debug!(self.logger,
+                                                                       "Encountered inconsistency during transaction sync, restarting.");
+                                                               sync_state.pending_sync = true;
                                                                continue;
                                                        }
@@ -145,7 +147,9 @@ where
                                                // Double-check the tip hash. If it changed, a reorg happened since
                                                // we started syncing and we need to restart last-minute.
-                                               if self.check_has_chain_moved(&mut sync_state, &mut tip_header,
-                                                       &mut tip_height)?
+                                               if self.check_update_tip(&mut tip_header, &mut tip_height)?
                                                {
+                                                       log_debug!(self.logger,
+                                                               "Encountered inconsistency during transaction sync, restarting.");
+                                                       sync_state.pending_sync = true;
                                                        continue;
                                                }
@@ -186,5 +190,7 @@ where
        }

-       fn check_has_chain_moved(&self, sync_state: &mut SyncState, cur_tip_header: &mut BlockHeader, cur_tip_height: &mut u32) -> Result<bool, InternalError> {
+       fn check_update_tip(&self, cur_tip_header: &mut BlockHeader, cur_tip_height: &mut u32)
+               -> Result<bool, InternalError>
+       {
                let check_notification = self.client.block_headers_subscribe()?;
                let check_tip_hash = check_notification.header.block_hash();
@@ -203,8 +209,4 @@ where
                        *cur_tip_header = check_notification.header;
                        *cur_tip_height = check_notification.height as u32;
-                       log_debug!(self.logger,
-                               "Encountered inconsistency during transaction sync, restarting.");
-                       sync_state.pending_sync = true;
-
                        Ok(true)
                } else {

lightning-transaction-sync/src/electrum.rs

tnull · 2023-11-16T11:07:38Z

Now included two more commits that set pending_sync when our last-minute tip checks failed and would have us error out (both for Esplora and Electrum).

lightning-transaction-sync/src/electrum.rs

tnull · 2023-11-22T09:16:08Z

I just double-checked that there are no version conflicts/notable API changes messing with this after the bumps in #2740. Have a rebased branch ready-to-go, so happy to land this after #2740.

TheBlueMatt

Should be good to go. I dunno if @jkczyz had any other thoughts, though.

We previously included the block hash, but it's also useful to include the height under which we expect the respective transaction to be confirmed.

We give some more information while reducing the log levels to make the logging less spammy. We also convert one safe-to-unwrap case from returning an error to unwrapping the value.

In particular, we now test `register_output` functionality, too.

tnull · 2023-11-23T08:33:53Z

Rebased on main after #2740 landed.

Now that we upgraded `esplora-client` to 0.6 we can use `async-https-rustls` instead of manually overriding the `reqwest` dependency.

tnull · 2023-11-24T11:50:55Z

Introduced a small follow-up commit to the rebase. As we've now upgraded to use esplora-client 0.6, we can make use of the introduced async-https-rustls feature.

tnull marked this pull request as draft October 25, 2023 14:40

tnull force-pushed the 2022-12-add-electrum-sync branch 3 times, most recently from eadc4ba to dfcc2f5 Compare October 27, 2023 10:28

tnull force-pushed the 2022-12-add-electrum-sync branch 4 times, most recently from c684ae0 to 75ad902 Compare October 30, 2023 09:07

tnull marked this pull request as ready for review October 30, 2023 09:13

tnull added the Seeking Code Review label Oct 30, 2023

tnull added this to the 0.0.119 milestone Oct 30, 2023

tnull force-pushed the 2022-12-add-electrum-sync branch from 75ad902 to e38abe0 Compare October 30, 2023 17:27

TheBlueMatt assigned jkczyz and TheBlueMatt Nov 5, 2023

jkczyz reviewed Nov 7, 2023

View reviewed changes

tnull force-pushed the 2022-12-add-electrum-sync branch from e38abe0 to 74cafd3 Compare November 7, 2023 09:11

jkczyz reviewed Nov 7, 2023

View reviewed changes

tnull force-pushed the 2022-12-add-electrum-sync branch from 74cafd3 to 5ba1c45 Compare November 8, 2023 10:38

jkczyz reviewed Nov 8, 2023

View reviewed changes

tnull force-pushed the 2022-12-add-electrum-sync branch from 5ba1c45 to ce31cf2 Compare November 8, 2023 15:50

jkczyz reviewed Nov 10, 2023

View reviewed changes

lightning-transaction-sync/src/electrum.rs Outdated Show resolved Hide resolved

lightning-transaction-sync/src/electrum.rs Outdated Show resolved Hide resolved

tnull force-pushed the 2022-12-add-electrum-sync branch from ce31cf2 to d9f6d41 Compare November 10, 2023 08:48

jkczyz previously approved these changes Nov 10, 2023

View reviewed changes

TheBlueMatt reviewed Nov 11, 2023

View reviewed changes

tnull dismissed jkczyz’s stale review via e14f83e November 13, 2023 10:29

tnull force-pushed the 2022-12-add-electrum-sync branch from d9f6d41 to e14f83e Compare November 13, 2023 10:29

jkczyz previously approved these changes Nov 15, 2023

View reviewed changes

TheBlueMatt reviewed Nov 15, 2023

View reviewed changes

lightning-transaction-sync/src/electrum.rs Outdated Show resolved Hide resolved

tnull dismissed jkczyz’s stale review via 85c0d77 November 16, 2023 10:58

tnull force-pushed the 2022-12-add-electrum-sync branch 2 times, most recently from 85c0d77 to c8dca25 Compare November 16, 2023 11:05

jkczyz reviewed Nov 16, 2023

View reviewed changes

lightning-transaction-sync/src/electrum.rs Outdated Show resolved Hide resolved

lightning-transaction-sync/src/electrum.rs Show resolved Hide resolved

tnull force-pushed the 2022-12-add-electrum-sync branch from c8dca25 to 36ade54 Compare November 16, 2023 14:12

jkczyz mentioned this pull request Nov 16, 2023

Group channel funding transaction fields (small refactor) [splicing] #2736

Closed

TheBlueMatt previously approved these changes Nov 22, 2023

View reviewed changes

tnull added 9 commits November 23, 2023 09:30

Return confirmation height via Confirm::get_relevant_txids

c8ff321

We previously included the block hash, but it's also useful to include the height under which we expect the respective transaction to be confirmed.

Use upstream TestLogger util in tx sync tests

5b7c1d5

Improve EsploraSyncClient logging

48c46bf

We give some more information while reducing the log levels to make the logging less spammy. We also convert one safe-to-unwrap case from returning an error to unwrapping the value.

Improve EsploraSyncClient test coverage

e14e2c3

In particular, we now test `register_output` functionality, too.

Move sync_ methods to SyncState

82f4c10

Set pending_sync when last-minute check fails in Esplora

d2ecf07

Implement ElectrumSyncClient

4dd5ccb

Add Electrum integration test

28c14cd

DRY up Esplora/Electrum integration_tests

c2e81fb

tnull dismissed TheBlueMatt’s stale review via c2e81fb November 23, 2023 08:33

tnull force-pushed the 2022-12-add-electrum-sync branch from 36ade54 to c2e81fb Compare November 23, 2023 08:33

Use esplora-client's async-https-rustls feature

31ea90e

Now that we upgraded `esplora-client` to 0.6 we can use `async-https-rustls` instead of manually overriding the `reqwest` dependency.

jkczyz approved these changes Nov 27, 2023

View reviewed changes

TheBlueMatt approved these changes Nov 27, 2023

View reviewed changes

TheBlueMatt merged commit fa0d015 into lightningdevkit:main Nov 27, 2023

tnull mentioned this pull request Nov 27, 2023

Bump MSRV to rustc 1.63.0 #2681

Merged

Add electrum support to lightning-transaction-sync #2685

Add electrum support to lightning-transaction-sync #2685

Uh oh!

Conversation

tnull commented Oct 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Oct 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tnull commented Oct 30, 2023

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tnull commented Nov 8, 2023

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tnull commented Nov 10, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tnull commented Nov 15, 2023

Uh oh!

Uh oh!

tnull commented Nov 16, 2023

Uh oh!

Uh oh!

Uh oh!

tnull commented Nov 22, 2023

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

tnull commented Nov 23, 2023

Uh oh!

tnull commented Nov 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Add electrum support to `lightning-transaction-sync` #2685

Add electrum support to `lightning-transaction-sync` #2685

tnull commented Oct 25, 2023 •

edited

Loading

codecov-commenter commented Oct 27, 2023 •

edited

Loading